-
Notifications
You must be signed in to change notification settings - Fork 395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Added utilization info for ECS #2565
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2565 +/- ##
=======================================
Coverage 97.22% 97.23%
=======================================
Files 290 291 +1
Lines 45882 45932 +50
=======================================
+ Hits 44608 44661 +53
+ Misses 1274 1271 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -51,6 +56,7 @@ test('pricing system-info aws', function (t) { | |||
|
|||
// This will throw an error if the sys info isn't being cached properly | |||
t.ok(awsRedirect.isDone(), 'should exhaust nock endpoints') | |||
t.ok(ecsScope.isDone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know you have unit tests but this integration test should be assertion the ecs vendor:
t.same(systemInfo.vendors.ecs, {
ecsDockerId: 'ecs-container-1'
})
lib/utilization/docker-info.js
Outdated
module.exports = { | ||
clearVendorCache: clearDockerVendorCache, | ||
getBootId, | ||
getEcsContainerId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the spec, I think the new solution should be replacing the original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This reverts commit 2ad7c42.
This PR resolves #2563.